-
Notifications
You must be signed in to change notification settings - Fork 323
feat: add support for alert auto-resolve + Incident.io integration #1298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add support for alert auto-resolve + Incident.io integration #1298
Conversation
🦋 Changeset detectedLatest commit: 3f3231c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code ReviewCritical Issues❌ Missing index for group-by queries → Add compound index { alert: 1, group: 1, createdAt: -1 } to AlertHistory model
Important Issues
Minor Observations✅ Test coverage is extensive (1845 lines added to checkAlerts.test.ts) RecommendationFix the missing compound index before merge - this is a performance issue that will impact production workloads with group-by alerts. |
E2E Test Results✅ All tests passed • 39 passed • 3 skipped • 304s
|
3e00bb0 to
fe1b1f6
Compare
packages/app/src/TeamPage.tsx
Outdated
| "title": "{{title}}", | ||
| "description": "{{body}}", | ||
| "deduplication_key": "{{eventId}}", | ||
| "status": "{{#if (eq state "ALERT")}}firing{{else}}resolved{{/if}}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f3a599e to
2119ef2
Compare
487e4a5 to
1a5b028
Compare
1a5b028 to
feb7176
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides the other comments, I also notice that since we create an alert history for each group, the Alerts page has an alert status per group, which is probably not correct:
For example, if there are three group keys, you'll get three red bars here for the same time and with different counts.
Just a thought - would it make more sense to save one alert history with all the groups, and maintain the state of each group within the one history entry? Sort of like how we have lastValues already? That could cut down on requests to mongo, help avoid making changes to the UI to fix the above ^^ issue, and maybe simplify some of the other code (particularly the stuff that handles partial mongo save failures)?
025dd4e to
2f828ae
Compare
2f828ae to
d3e29ad
Compare
d3e29ad to
2a3adce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with the understanding that there are a few followup items listed in various comments
Thanks. I will file tickets for those followup items |
Plus fixed 'group-by' alert state issues (alert histories)
Ref: HDX-2661
Ref: HDX-2660